Skip to content

feat(sdk-core): pass optional attestation through multisig send/initiate#9216

Open
mr-neptune wants to merge 1 commit into
masterfrom
WCN-539/withdrawal-flow-attestation-UI
Open

feat(sdk-core): pass optional attestation through multisig send/initiate#9216
mr-neptune wants to merge 1 commit into
masterfrom
WCN-539/withdrawal-flow-attestation-UI

Conversation

@mr-neptune

@mr-neptune mr-neptune commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Add a local AttestationPayload codec and thread it through wallet.sendMany's build/send/initiate path so a WebAuthn attestation survives to POST /tx/send and /tx/initiate: appended to BuildParams (clears the sendMany prebuild-whitelist pick), to whitelistedSendParams, and re-added via a local t.intersection partial since @bitgo/public-types' TxSendBody (t.exact) doesn't declare the field yet and would strip it on encode. Pure pass-through — no attestation validation in the SDK.

Scoped to what WCN-539 (UI) needs end-to-end: multisig only. The shared upstream codec change and the /txrequests, /signatureshares, and /pendingapprovals endpoints are left as TODO(WCN-541) markers for the dedicated SDK/Express pass-through ticket.

Ticket: WCN-539

@mr-neptune
feat(sdk-core): pass optional attestation through multisig send/initiate
f3a75cc 
Add a local AttestationPayload codec and thread it through
wallet.sendMany's build/send/initiate path so a WebAuthn attestation
survives to POST /tx/send and /tx/initiate: appended to BuildParams
(clears the sendMany prebuild-whitelist pick), to whitelistedSendParams,
and re-added via a local t.intersection partial since @bitgo/public-types'
TxSendBody (t.exact) doesn't declare the field yet and would strip it on
encode. Pure pass-through — no attestation validation in the SDK.

Scoped to what WCN-539 (UI) needs end-to-end: multisig only. The shared
upstream codec change and the /txrequests, /signatureshares, and
/pendingapprovals endpoints are left as TODO(WCN-541) markers for the
dedicated SDK/Express pass-through ticket.

Ticket: WCN-539
@mr-neptune mr-neptune requested review from a team as code owners July 8, 2026 23:06
@mr-neptune mr-neptune requested a review from rajangarg047 July 8, 2026 23:06
@linear-code

linear-code Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

WCN-539

@mr-neptune mr-neptune requested a review from Marzooqa July 8, 2026 23:06
@Marzooqa

Marzooqa commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@claude

@github-actions

github-actions Bot commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Claude finished @Marzooqa's task —— View job

PR Review Analysis

Todo List:

  • Set up repository and install dependencies
  • Read and analyze all changed files
  • Review AttestationPayload codec implementation
  • Check threading through wallet.sendMany path
  • Analyze test coverage and implementation
  • Provide comprehensive review feedback

Review Findings

✅ Architecture & Implementation

The PR correctly implements pass-through attestation for WebAuthn withdrawal flows. Key strengths:

  1. Clean codec design: AttestationPayload is well-defined with clear field requirements
  2. Proper threading: Attestation flows correctly through BuildParams → whitelistedSendParams → TxSendBody intersections
  3. Scoped approach: Appropriately limited to multisig-only for WCN-539 with clear TODO markers for future expansion

✅ Code Quality

  • Strong TypeScript typing with io-ts codecs
  • Comprehensive test coverage in both BuildParams.ts and SendTransactionRequest.ts
  • Good documentation with clear comments explaining the temporary nature of local codecs

⚠️ Areas for Consideration

1. Codec Duplication Pattern
The PR uses local t.intersection with AttestationPayload in both sendTransaction and initiateTransaction:

// wallet.ts:5072-5075 and 5088-5091
t.intersection([TxSendBody, t.partial({ attestation: AttestationPayload })])

Suggestion: Consider extracting this to a shared codec to reduce duplication and ensure consistency.

2. Documentation Clarity
While the TODO comments are helpful, consider adding more explicit documentation about:

  • The expected format/validation of attestation fields
  • Error handling when attestation is malformed
  • Security considerations for attestation validation (even if SDK doesn't validate)

3. Test Coverage Gap
The tests verify codec behavior but don't test the actual flow through sendMany. Consider adding integration tests that verify attestation survives the full sendMany → sendTransaction path.

✅ Security & Standards

  • Proper handling of WebAuthn attestation structure
  • No sensitive data exposure
  • Follows existing SDK patterns for parameter whitelisting

Minor Suggestions

BuildParams.ts:75-80: The AttestationPayload fields match WebAuthn standards correctly.

wallet.ts:148: The whitelistedSendParams array modification is clean and follows existing patterns.

Tests: Strong coverage of edge cases, particularly the codec validation tests.

Verdict: APPROVED ✅

This is a well-implemented, narrowly scoped change that correctly adds attestation pass-through without breaking existing functionality. The temporary codec workarounds are appropriately documented and will be cleaned up in WCN-541.

🔗 Job Run | 🌿 Branch: WCN-539/withdrawal-flow-attestation-UI

OttoAllmendinger
OttoAllmendinger requested changes Jul 9, 2026
View reviewed changes
Comment thread modules/sdk-core/test/unit/bitgo/wallet/SendTransactionRequest.ts
});
});

it('TxSendBody alone drops attestation (upstream codec has no such field yet — TODO(WCN-541))', function () {

@OttoAllmendinger OttoAllmendinger Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's just fix the upstream codec first instead of hacking around it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@OttoAllmendinger OttoAllmendinger OttoAllmendinger requested changes
@rajangarg047 rajangarg047 Awaiting requested review from rajangarg047 rajangarg047 is a code owner automatically assigned from BitGo/wallet-core
@Marzooqa Marzooqa Awaiting requested review from Marzooqa Marzooqa is a code owner automatically assigned from BitGo/wallet-core-india

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mr-neptune @Marzooqa @OttoAllmendinger